Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed missing BlankProject #647

Closed
wants to merge 2 commits into from
Closed

Fixed missing BlankProject #647

wants to merge 2 commits into from

Conversation

homapf
Copy link
Contributor

@homapf homapf commented Apr 18, 2024

Changes

  • Removed creation and deletion of BlankProject on Mac runners

Deleting the BlankProject on cleanup creates the following issue when calling the action multiple times in a row:

Couldn't set project path to: 

Aborting batchmode due to failure:
Couldn't set project path to: 

[Package Manager] Server::Kill -- Server was shutdown
Unclassified error occured while trying to activate license.
Exit code was: 1
Error: There was an error while trying to activate the Unity license.
Error: Build failed with exit code 1

There is no need to create the BlankProject as it already exists in the action.

Successful Workflow Run Link

This only affects self-hosted mac runners. Cannot share a workflow run here unfortunately.

Checklist

  • Read the contribution guide and accept the
    code of conduct
  • Docs (If new inputs or outputs have been added or changes to behavior that should be documented. Please make a PR
    in the documentation repo)
  • Readme (updated or not needed)
  • Tests (added, updated or not needed)

@github-actions GitHub Actions
Copy link

Cat Gif

GabLeRoux
GabLeRoux approved these changes Apr 25, 2024
View reviewed changes
Copy link
Member

@GabLeRoux GabLeRoux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I'm wondering if the same problem happens on the test runner action.

@homapf
Copy link
Contributor Author

homapf commented Jul 26, 2024

Any blocker to merge @GabLeRoux ?

I'd gladly look at the test runner action if I have some time.

@webbertakken
Copy link
Member

webbertakken commented Oct 7, 2024 •
edited
Loading

The code that's being removed here doesn't fail by itself.

  • mkdir -p "$ACTIVATE_LICENSE_PATH" has the -p flag that makes sure it doesn't fail if the folder already exists.
  • rm -r "$ACTIVATE_LICENSE_PATH" will never fail because the folder is guaranteed to exist.

The change you're proposing doesn't actually fix any error as far as I can see. What am I missing?

Please also note that this way of activation was added because it is needed for the normal activation workflow and can not simply be deleted.

I see a single thumbs up and no other people have reported this same problem. I'm going to have to close this in favour of a different kind of fix if any. Are you still running into a problem with this @homapf?

@webbertakken
Copy link
Member

cc: @GabLeRoux

@GabLeRoux
Copy link
Member

I agree; removing those two lines doesn’t seem to address what’s mentioned in the description. If this issue persists, let’s open a discussion to confirm the right approach.

It’s possible we might be overlooking something, but I don’t currently see how these changes resolve what’s outlined. Let us know if we can help. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GabLeRoux GabLeRoux GabLeRoux approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@homapf @webbertakken @GabLeRoux